-
Notifications
You must be signed in to change notification settings - Fork 160
[Preview] Rewards Eligibility Oracle (REO) #1235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: build-lint-upgrade
Are you sure you want to change the base?
Conversation
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new Rewards Eligibility Oracle (REO) system that allows authorized oracles to manage indexer eligibility for receiving rewards on The Graph network. The system implements a "deny by default" approach where indexers must be explicitly marked as eligible by oracles to receive rewards.
Key changes:
- New RewardsEligibilityOracle contract with time-based eligibility management
- Integration with existing RewardsManager to check eligibility before distributing rewards
- Comprehensive test suite with consolidated patterns and shared fixtures
Reviewed Changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
packages/issuance/contracts/eligibility/RewardsEligibilityOracle.sol | Core oracle contract implementing eligibility management with role-based access control |
packages/contracts/contracts/rewards/RewardsManager.sol | Updated to integrate with eligibility oracle and check indexer eligibility before rewards |
packages/interfaces/contracts/issuance/eligibility/IRewardsEligibilityOracle.sol | Interface definition for rewards eligibility oracle |
packages/issuance/test/tests/RewardsEligibilityOracle.test.ts | Comprehensive test suite for the oracle contract |
packages/contracts/test/tests/unit/rewards/rewards.test.ts | Enabled previously skipped eligibility oracle tests |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
[Preview] Rewards Eligibility Oracle (REO)
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## build-lint-upgrade #1235 +/- ##
======================================================
+ Coverage 83.15% 83.69% +0.53%
======================================================
Files 48 51 +3
Lines 2096 2183 +87
Branches 620 643 +23
======================================================
+ Hits 1743 1827 +84
- Misses 353 356 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
• Consolidated duplicate test fixture files into single fixtures.ts • Removed circular dependency between fixtures.ts and sharedFixtures.ts • Fixed incorrect function calls (grantOperatorRole → grantRole, setValidityPeriod → setEligibilityPeriod) • Updated default eligibility period from 7 days to 14 days (matches contract default) • Updated all test imports to use consolidated fixtures • Fixed RewardsManagerV6Storage class documentation comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 33 out of 35 changed files in this pull request and generated 7 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/issuance/test/tests/consolidated/AccessControl.test.ts
Outdated
Show resolved
Hide resolved
• Convert require() to ES6 imports for consistency (chai, ethers) • Add descriptive comments to empty catch blocks in generateInterfaceIds.js • Add mocha configuration to hardhat.config.cjs • Remove commented-out paths configuration • Add test:coverage script to package.json • Improve error handling in fixtures.ts (console.warn → console.error + throw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 34 out of 35 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…tance - Convert from CommonJS to TypeScript for consistency - Create issuance-specific base config that inherits from toolshed - Remove unnecessary configuration duplication: - Remove unused hardhat-abi-exporter, hardhat-gas-reporter, solidity-coverage - Remove unnecessary mocha config (tests run in separate package) - Remove redundant etherscan customChains (arbitrumSepolia is built-in) - Remove manual etherscan API key config (inherited from toolshed) - Set evmVersion to 'cancun' for issuance contracts - Eliminate ~95% duplication across 3 config files - Follow established inheritance pattern like horizon/subgraph-service Before: 289 lines across 3 files with massive duplication After: ~90 lines with proper inheritance and minimal duplication
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
- Updates issuance package from 5.3.0 to 5.4.0 (latest stable) - Updates issuance test package from 5.3.0 to 5.4.0 - Maintains compatibility with Solidity 0.8.27 - All tests passing with updated dependencies
…uilding - Add timestamp-based checks for WAGMI, ethers-v5, and TypeScript compilation - Skip type generation when output files are newer than source files - Improve file-by-file comparison for dist directory organization - Reduce build time from 5+ seconds to ~600ms when no changes needed - Maintain build correctness while improving developer experience
The testSlash_RoundDown_TokensThawing_Delegation test was failing because it could generate scenarios where undelegating would leave less than the minimum delegation amount (1 ether) in the pool, which violates the protocol's minimum delegation constraint. Added assumption to ensure undelegation either removes all tokens or leaves at least MIN_DELEGATION in the pool, making the test compliant with the protocol's validation rules introduced in commit 91cda56.
- Update test script to use main hardhat config instead of test-specific config - Remove unused test/hardhat.config.ts file - Fixes OpenZeppelin upgrades plugin contract resolution issue - All 40 RewardsEligibilityOracle tests now pass Resolves issue introduced in 39b161c where test config couldn't properly resolve contract sources for OpenZeppelin upgrades plugin validation.
The build:dep step already compiles @graphprotocol/issuance as a dependency, so the explicit --filter @graphprotocol/issuance compile in build:self was redundant and inefficient. Now follows the same pattern as packages/contracts/test which only builds dependencies without redundant compilation steps.
- Remove 'html' from istanbulReporter in .solcover.js files - Keep only ['lcov', 'text', 'json'] reporters to avoid duplicates - HTML reports now only generated in lcov-report/ subdirectory - Use default ./coverage output directory for modern pattern - Apply fix to both contracts and issuance packages
- Replace manual environment variable management with hre.__SOLIDITY_COVERAGE_RUNNING - Add centralized isRunningUnderCoverage() utility function - Simplify test coverage detection using clean if statements - Remove duplicate .solcover.js file (keep only test/.solcover.js) - Update coverage configuration for better reporting
Update CI to look for coverage files in packages/contracts/coverage/ instead of packages/contracts/test/reports/coverage/ to match the updated .solcover.js configuration.
Currently comparing to build-lint-upgrade, when that merged to main (via #1233) can create as new PR against main for easier audit and merge.